Add a command to purge the relay chain only by cecton · Pull Request #306 · paritytech/cumulus

cecton · 2021-01-22T07:57:17Z

Fixes #259

cecton · 2021-01-22T07:57:36Z

Tested and working:

[0] [07:55:34] ~/r/cumulus cecton-purge-relay > ./target/release/rococo-collator purge-chain --base-path /tmp/rococo
Are you sure to remove "/tmp/rococo/chains/local_testnet/db"? [y/N]: y
"/tmp/rococo/chains/local_testnet/db" removed.
[0] [07:55:43] ~/r/cumulus cecton-purge-relay > ./target/release/rococo-collator purge-relay-chain --base-path /tmp/rococo
Are you sure to remove "/tmp/rococo/polkadot/chains/westend_dev/db"? [y/N]: y
"/tmp/rococo/polkadot/chains/westend_dev/db" removed.

bkchr · 2021-01-22T09:20:26Z

Commented on the issue

cecton · 2021-01-22T11:42:19Z

[0] [11:29:44] ~/r/cumulus cecton-purge-relay > ./target/debug/rococo-collator purge-chain --parachain
/home/cecile/.local/share/rococo-collator/chains/local_testnet/db
Are you sure to remove? [y/N]: ^C⏎
[130] [11:36:24] ~/r/cumulus cecton-purge-relay > ./target/debug/rococo-collator purge-chain
/home/cecile/.local/share/rococo-collator/chains/local_testnet/db
/home/cecile/.local/share/rococo-collator/polkadot/chains/westend_dev/db
Are you sure to remove? [y/N]: ^C⏎
[130] [11:36:30] ~/r/cumulus cecton-purge-relay > ./target/debug/rococo-collator purge-chain --relay
/home/cecile/.local/share/rococo-collator/polkadot/chains/westend_dev/db
Are you sure to remove? [y/N]: ^C⏎
[130] [11:36:34] ~/r/cumulus cecton-purge-relay > ./target/debug/rococo-collator purge-chain --relay --para
/home/cecile/.local/share/rococo-collator/chains/local_testnet/db
/home/cecile/.local/share/rococo-collator/polkadot/chains/westend_dev/db
Are you sure to remove? [y/N]: y
"/home/cecile/.local/share/rococo-collator/chains/local_testnet/db" did not exist.
"/home/cecile/.local/share/rococo-collator/polkadot/chains/westend_dev/db" did not exist.

bkchr

Please move and also please write some tests for the command on its own.

rococo-parachains/src/command.rs

rococo-parachains/src/cli.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

bkchr

(and please don't forget the tests 🙈 )

cli/src/lib.rs

cecton · 2021-01-22T14:57:21Z

(and please don't forget the tests see_no_evil )

I'm on it. You're too fast

bkchr · 2021-01-22T15:33:26Z

(and please don't forget the tests see_no_evil )

I'm on it. You're too fast

Sorry :P

cecton · 2021-01-22T19:46:25Z

There is an issue with the --dev flag. There is one for the para chain, one for the relay chain. For some reasons (limitation in structopt or maybe clap) I have problems to retrieve the parameters after -- for the purge-chain subcommand... and that is a big problem to solve.

The other problem with this is that it is also impacting the --chain. The only way I see right now would be to add parameters --relay-dev and --relay-chain to the cumulus purge-chain command.

Meanwhile I have a few questions:

Are we sure we need to support polkadot args in the purge-chain command?
Do I need to use the --dev flag at all in the tests? Before it was required to avoid connecting the outside world I think but now when I look at the example in the README and we don't use --dev at all anywhere.

One other (very far fetched) alternative would be to give a big thought about moving away from structopt and changing completely sc-cli's API to use clap directly. It's less declarative but this is actually a good thing in term of flexibility. We might be able to achieve more with less complexity.

bkchr · 2021-01-25T13:20:12Z

There is an issue with the --dev flag. There is one for the para chain, one for the relay chain. For some reasons (limitation in structopt or maybe clap) I have problems to retrieve the parameters after -- for the purge-chain subcommand... and that is a big problem to solve.

The other problem with this is that it is also impacting the --chain. The only way I see right now would be to add parameters --relay-dev and --relay-chain to the cumulus purge-chain command.

--dev is just a shortcut for --chain something. So, IMHO we could omit this here because it doesn't make any sense.

Meanwhile I have a few questions:

* Are we sure we need to support polkadot args in the purge-chain command?

* Do I need to use the `--dev` flag at all in the tests? Before it was required to avoid connecting the outside world I think but now when I look at the example in the README and we don't use --dev at all anywhere.

You should make yourself familiar what this command is doing. From the perspective of cumulus the --dev never worked, but we could probably make it work. As explained above, the --dev is just a shortcut for --chain bla and you can use that in your tests.

One other (very far fetched) alternative would be to give a big thought about moving away from structopt and changing completely sc-cli's API to use clap directly. It's less declarative but this is actually a good thing in term of flexibility. We might be able to achieve more with less complexity.

We used clap before and it was horrible :P Yeah, it is more flexible, but you also loose all your shit when you try to modify something or to add a new parameter. So, no switch.

cecton · 2021-01-29T10:56:02Z

As explained above, the --dev is just a shortcut for --chain bla and you can use that in your tests.

I know that very well but my problem is still there. I will try to explain better:

When you run cumulus you can give a --chain to the relay chain and another one to the para chain. It works because of how things are parse in the command line.

Unfortunately, because of limitations with structopt, I can't parse the arguments after -- when using the purge command. Because of that, the test fails because the process is first started with one chain (dev) but the purge command (ran later on during the test) is using another chain (westend-dev).

What I can do to circumvent this issue:

Add a new parameter --relay-chain to the purge command
Store the chain name in a file in the base path of the para chain (so it can locate the directory)

(I'm mostly using the parameter --dev in the test to avoid the nodes to connect to anything.)

cecton · 2021-01-29T11:11:20Z

Add a new parameter --relay-chain to the purge command

Wait, there is already a parameter --relay-chain. I meant a parameter --relay-chain-chain <chain-file> (probably with a better name lol)

cecton · 2021-01-29T11:12:50Z

Wait, there is already a parameter --relay-chain. I meant a parameter --relay-chain-chain <chain-file> (probably with a better name lol)

Or maybe just make --relaychain takes an optional path argument to the chain file

This reverts commit f97cd63.

cecton · 2021-01-29T11:59:44Z

Ok I get it, the relay chain is actually found from the parachain's extension. So I just fixed the test. @bkchr this is ready for review then

bkchr

Sorry for the late review. Some last nitpicks, otherwise looks good.

cli/src/lib.rs

bkchr · 2021-02-27T17:49:05Z

cli/src/lib.rs

+			.iter()
+			.map(|x| {
+				x.path().ok_or_else(|| {
+					sc_cli::Error::Input("Cannot purge custom database implementation".into())


Maybe a hint which on this is, would be nice :D

Done in 670b674

cli/src/lib.rs

bkchr · 2021-02-27T17:51:49Z

rococo-parachains/tests/purge_chain_works.rs


 #[test]
 #[cfg(unix)]
 fn purge_chain_works() {


Would be really nice to have these tests part of the /tests/ "parachain". But this can be done in a later pr.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

JoshOrndorff · 2021-04-13T19:42:47Z

rococo-parachains/tests/purge_chain_works.rs

+		};

-	let base_path = "purge_chain_test";
+		let base_path = tempfile::tempdir().unwrap();


@cecton I'm curious why you did this after you already gave us the --tmp flag in paritytech/substrate#6221

Oh, I get it. So you can check the directory later on.

Add a command to purge the relay chain only

30b881a

cecton marked this pull request as draft January 22, 2021 09:48

WIP

137a2b0

cecton marked this pull request as ready for review January 22, 2021 11:40

cecton mentioned this pull request Jan 22, 2021

Subcommand to purge embedded relay chain #259

Closed

bkchr reviewed Jan 22, 2021

View reviewed changes

rococo-parachains/src/command.rs Outdated Show resolved Hide resolved

rococo-parachains/src/cli.rs Outdated Show resolved Hide resolved

rococo-parachains/src/cli.rs Outdated Show resolved Hide resolved

cecton and others added 3 commits January 22, 2021 15:06

Update rococo-parachains/src/cli.rs

552676b

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Move cli stuff to its own crate

358c18c

Copyright dates

f60c5df

bkchr reviewed Jan 22, 2021

View reviewed changes

cli/src/lib.rs Show resolved Hide resolved

Test not working for some reason...

087c098

cecton marked this pull request as draft January 29, 2021 09:05

WIP

f97cd63

cecton added 4 commits January 29, 2021 12:28

Revert "WIP"

d5179ff

This reverts commit f97cd63.

Fix test to use provided relay chain

c288c7b

Merge commit b5f6580 (conflicts)

db85797

Merge commit 0d086ae (no conflict)

53d759e

cecton marked this pull request as ready for review January 29, 2021 11:58

Merge commit cb00749 (no conflict)

adc96eb

Merge commit 6438d32 (conflicts)

b9f3c66

bkchr approved these changes Feb 27, 2021

View reviewed changes

cecton mentioned this pull request Feb 28, 2021

Move purge chain tests to the /tests/ "parachain" #347

Open

cecton and others added 7 commits February 28, 2021 10:56

Apply suggestions from code review

060b186

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Add hint about which database could not be purged

670b674

Merge commit e065c57 (no conflict)

a258791

Merge The Great Refactor fbacfe7 (conflicts)

6d27365

Merge commit ec08d11 (no conflict)

e1090da

Merge commit 4820fa1 (conflicts)

ff2a80f

Merge commit f511757 (no conflict)

5cf0be0

bkchr merged commit 5cdbd7c into master Mar 3, 2021

bkchr deleted the cecton-purge-relay branch March 3, 2021 12:40

Satoshi-Kusumoto mentioned this pull request Mar 25, 2021

Update cumulus litentry/heima#38

Merged

This was referenced Apr 5, 2021

Allow purging relay and para separately moonbeam-foundation/moonbeam#329

Merged

cumulus-client-cli should re-export from sc-cli #389

Open

JoshOrndorff reviewed Apr 13, 2021

View reviewed changes

Conversation

cecton commented Jan 22, 2021

Uh oh!

cecton commented Jan 22, 2021

Uh oh!

bkchr commented Jan 22, 2021

Uh oh!

cecton commented Jan 22, 2021

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cecton commented Jan 22, 2021

Uh oh!

bkchr commented Jan 22, 2021

Uh oh!

cecton commented Jan 22, 2021

Uh oh!

bkchr commented Jan 25, 2021

Uh oh!

cecton commented Jan 29, 2021

Uh oh!

cecton commented Jan 29, 2021

Uh oh!

cecton commented Jan 29, 2021

Uh oh!

cecton commented Jan 29, 2021

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkchr Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

cecton Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkchr Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

JoshOrndorff Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

JoshOrndorff Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants